Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

lsp-ws-connection in repo (follow up) #108

Merged
merged 4 commits into from
Dec 10, 2019
Merged

lsp-ws-connection in repo (follow up) #108

merged 4 commits into from
Dec 10, 2019

Conversation

bollwyvl
Copy link
Collaborator

References

Code changes

  • Use relative file:../<pkg> links in metapackage dependencies.
  • Use extremely loose * dependency between jupyterlab-lsp and lsp-ws-connection for now
    • we'll have to tighten this up when we start making substantive changes, and certainly at release
    • it may well have not been getting used previously... luckily it still appears to work 😝
  • remove watch from jupyterlab-lsp (so there's only the one tsc -w running in metapackage, and the one webpatch --watch running in lsp-ws-connection)
    • this probably wasn't creating a problem... yet... but was contributing even more to the watched/open file limit, which is probably very real threat now that we have lsp-ws-connection in there. I'm glad I'm not developing on windows.

I do recommend a jupyter lab clean, and full run of postBuild to ensure everything has been cleaned right after we add a new sub-package (hopefully not too many more for a while). Also, due to the wackiness of the webpack build(s), there can be some thrashing after a change to lsp-ws-connection as built by jlpm watch and jupyter lab --watch... i usually end up making a few whitespace changes and then reverting them so it catches up (one way or the other). Not ideal, but there's kind of a lot going on down in there, and having to do a whole release to try out a new thing Is Not Fun.

User-facing changes

None

Backwards-incompatible changes

None

Chores

  • linted
  • tested
  • documented (nothing yet)
  • changelog entry (nothing yet)

@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@bollwyvl
Copy link
Collaborator Author

Also, by fixing the dependencies, the lsp-ws-connection tests get run first by jlpm test (it does a topological sort for serial commands). Neat! That it wasn't before should have been a tip-off that something wasn't right. 🤷‍♀️

@bollwyvl
Copy link
Collaborator Author

Test reporting wouldn't have caught anything in particular, but it's nice to have and luckily didn't pull in too many additional dependencies.

@bollwyvl bollwyvl requested a review from krassowski December 10, 2019 02:30
krassowski
krassowski reviewed Dec 10, 2019
View reviewed changes
packages/metapackage/package.json
"@krassowski/jupyterlab-lsp": "^0.6.1",
"lsp-ws-connection": "^0.2.0"
"@krassowski/jupyterlab-lsp": "file:../jupyterlab-lsp",
"lsp-ws-connection": "file:../lsp-ws-connection"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was thinking about it but feared the outcome for the publishing process... But I guess it's better to start with than nothing. I will read up on how JupyterLab solves this (sure someone from the team may have some ideas on how to do it better as well).

krassowski
krassowski reviewed Dec 10, 2019
View reviewed changes
yarn.lock
dependencies:
vscode-jsonrpc "^4.0.0"
vscode-jsonrpc "^4.1.0-next"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note on next in here.

@krassowski krassowski merged commit 11d536d into jupyter-lsp:master Dec 10, 2019
@krassowski
Copy link
Member

It's much better now, I see that all the files in node_modules in my real filesystem are ok, but the webpack is just reluctant to update to reflect the changes - unless I manually restart jupyter lab --watch (obviously not ideal).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krassowski krassowski krassowski approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bollwyvl @krassowski